Build in debug mode for PRs #1375
Conversation
…ding the wheel artifacts from build stage
…pushes to main and release or candidate tags.
…s erasing the other wheel
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors CI to use reusable build/test workflows and to run PR builds in debug mode for faster feedback while keeping release-mode builds for main and tags.
Changes:
- Split CI into reusable workflows (
build.yml,test.yml) and added dedicated entry workflows for PRs (ci.yml) and releases (release.yml). - Updated test workflow to install and test against the pre-built Linux wheel artifact instead of rebuilding during tests.
- CI/config formatting cleanups across TOML files and adjusted license generation to rely on
cargo-license.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Formatting cleanup and a dependency list syntax fix (trailing comma). |
| examples/datafusion-ffi-example/Cargo.toml | TOML formatting cleanup for pyo3 features list. |
| examples/datafusion-ffi-example/.cargo/config.toml | TOML formatting cleanup for rustflags. |
| dev/create_license.py | Switches license generation to call cargo-license directly. |
| .github/workflows/test.yml | Converts to reusable workflow and installs pre-built wheel artifacts for tests. |
| .github/workflows/release.yml | Adds release entry workflow calling reusable build/test in release mode. |
| .github/workflows/ci.yml | Adds PR entry workflow calling reusable build/test in debug mode. |
| .github/workflows/build.yml | Converts to reusable workflow; adds linting jobs and reworks wheel build/artifact behavior. |
| .cargo/config.toml | TOML formatting cleanup for rustflags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
davisp
left a comment
davisp
left a comment
There was a problem hiding this comment.
+1
Overall everything looks fine and is straightforward. The only thought I had was the the build.yml workflows could maybe be condensed into a single parameterized matrix. Though that means that all the individual steps would end up requiring more parameterization which might turn out even more complicated than just repeated the same basic job multiple times.
| branches: | ||
| - "main" | ||
| tags: | ||
| - "*-rc*" # Release candidates (e.g., 45.0.0-rc1) |
There was a problem hiding this comment.
Should we add the [0-9]+ prefix here?
| "-C", "link-arg=-undefined", | ||
| "-C", "link-arg=dynamic_lookup", | ||
| ] | ||
| rustflags = ["-C", "link-arg=-undefined", "-C", "link-arg=dynamic_lookup"] |
There was a problem hiding this comment.
Not sure if we care, but near as I can tell, everything below here is just reformatting code.
There was a problem hiding this comment.
I added taplo to the checks is all, which requires reformatting
| with: | ||
| target: x86_64-unknown-linux-gnu | ||
| manylinux: "2_28" | ||
| args: --release --strip --features protoc,substrait --out dist |
There was a problem hiding this comment.
Not sure if its important. but the linux wheels are built with the protoc feature enabled, but the windows and mac wheels don't include it.
There was a problem hiding this comment.
This is intentional. The linux builds use the protoc feature but that can fail the mac/windows builds so we use system proto.
3 participants
This PR is to improve our CI performance in a couple of ways